Skip to content

Conversation

@senceryazici
Copy link
Contributor

@senceryazici senceryazici commented Feb 14, 2022

Hi,
I've configured ROS industrial_ci (which is almost standard for building ros workspaces) for you to build dave. Also added cmake args for building with lld as linkerscript (faster than ld).

Please note that, even though the dave repository is included in dave_sim.repos (causing CI to build & test it twice), it builds faster due to lld. industrial_ci will build dave as upstream_ws first (since it's in dave_sim.repos ), and then will build as target_ws since it's the actual repository, I have not removed dave repository from dave_sim.repos. You can either remove it from there (preffered), or create another .repos files without dave, to make CI even faster (about 17 mins.)

Also, .repos files with ssh links ( such as git@github:...) will fail in industrial_ci. I've added a sed command to replace them with https in this line.

@mabelzhang mabelzhang requested a review from M1chaelM February 18, 2022 00:29
@mabelzhang
Copy link
Contributor

mabelzhang commented Feb 18, 2022

I edited the post so that the links point to the correct URL for the dave_sim.repos file.

@M1chaelM
Copy link
Contributor

@senceryazici Thanks for doing this! It looks like an improvement to me and I'm ready to try it out, but I have a question about your change to the code_check.sh script. I leave you a comment inline.

elif [ $QUICK_CHECK -eq 0 ]; then
echo $CPPLINT_FILES | xargs python tools/cpplint.py 2>&1
fi
echo ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@senceryazici What does this addition do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is definitely harmless, I'm going to go ahead and merge, but if you can explain I'd appreciate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing on my local with 'act' (a tool for running GitHub actions on your local) the code check script constantly failed for some reason. Just for debugging I added a line to the end to see the result and it started passing.

Honestly (just like you've said) since it's totally harmless I didn't spend extra time on it's reason and let it be that way. But I'll definitely take a look at it when I have available time.

Copy link
Contributor

@M1chaelM M1chaelM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, and we're going to try it out. It sounds like removing the dave repos from dave_sim.repos would ultimately make sense also, though we will do that later just for logistical reasons relating to our upcoming release.

elif [ $QUICK_CHECK -eq 0 ]; then
echo $CPPLINT_FILES | xargs python tools/cpplint.py 2>&1
fi
echo ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is definitely harmless, I'm going to go ahead and merge, but if you can explain I'd appreciate it.

@M1chaelM M1chaelM merged commit 34a78e4 into Field-Robotics-Lab:master Feb 25, 2022
@bsb808
Copy link
Contributor

bsb808 commented Feb 25, 2022

This looks good, and we're going to try it out. It sounds like removing the dave repos from dave_sim.repos would ultimately make sense also, though we will do that later just for logistical reasons relating to our upcoming release.

@M1chaelM If you haven't done it yet, could you post an issue to the tracker to describe what needs to be done so that we have it on our list for post release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants